Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Dynamic base liquidation percentage #539

Merged
merged 8 commits into from
Mar 24, 2020
Merged

Dynamic base liquidation percentage #539

merged 8 commits into from
Mar 24, 2020

Conversation

NicholasDotSol
Copy link
Contributor

@NicholasDotSol NicholasDotSol commented Mar 23, 2020

Replace AUCTION_BASE_PERCENTAGE constant with a function that returns base percentage given the initialCollateralizationPercent from tbtcSystem

The base collateralization percentage should be the percentage of the InitialCollateralizationPercent that will result in a 100% bond value base auction assuming perfect collateralization.

  • Note, this breaks down if the recorded collateralization percentage is below 100%

But why

Starting auctions assuming proper collateralization reduces the attractiveness attacks where a liquidator can get ETH for a discount by manipulating oracles to initiate liquidation on a well-collateralized deposit.

NicholasDotSol added 5 commits March 23, 2020 15:06
To replace the removed constant,
getAuctionBasePercentage
calculates the current base auction percentage
such that given sorrect collateralization
the base will be 100% of the backing value

eg: collateralization:
500 collateral percent-> 20% base auction duration
150 collateral percent -> 66% base auction duration
100 collateral percent -> 100%
base auction duration

Note: This breaks when we go under 100% collateralization
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few notes, main one is: we now have to store that initial collateralization percentage :/

/// in a 100% bond value base auction given perfect collateralization.
function getAuctionBasePercentage(Deposit storage _d) internal view returns (uint256) {
ITBTCSystem _sys = ITBTCSystem(_d.TBTCSystem);
uint256 initialCollateralizedPercent = _sys.getInitialCollateralizedPercent();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to store this at deposit creation, otherwise the existing deposit's behavior changes based on governance actions :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

derp.


const initialCollateralization = await tbtcSystemStub.getInitialCollateralizedPercent()

// 10000 to avoid losing value to truncating is solidity.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in?

const initialCollateralization = await tbtcSystemStub.getInitialCollateralizedPercent()

// 10000 to avoid losing value to truncating is solidity.
const expecged = new BN(10000).div(initialCollateralization)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sp

NicholasDotSol added 2 commits March 24, 2020 00:05
Using current system initialCollateralizedPercent
does not correctly reflect the individual Deposit's
initialCollateralizedPercent.

Track initialCollateralizedPercent for each
individual Deposit
@NicholasDotSol
Copy link
Contributor Author

@Shadowfiend, ready here

@@ -33,6 +33,7 @@ library DepositUtils {
uint256 lotSizeSatoshis;
uint8 currentState;
uint256 signerFeeDivisor;
uint128 initialCollateralizedPercent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the two below seem like they can be smaller than uint128, perhaps? Could even be a uint8? Maybe a uint16 if we want to allow ourselves maximum flexibility, but our percentages are expressed as percentages, so uint8 gives us up to 250% as a representation 🤔

Not sure if the Solidity compiler is good enough to pack things together and save us gas in that case though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uint16 for some peace of mind. Also, the enforced initialCollateralizedPercent max is 300, which is over the 255 uint8 limit.
Solidity will automatically pack where possible, given the variables fit into a single 32-byte increment. two uint128s should be packed

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this looks good; let's handle variable sizing in a separate PR and merge this in the meantime.

@Shadowfiend
Copy link
Contributor

Putting a hold here as @mhluongo wants to leave <100% collateralization as an available initial percentage, so we need to see how that affects decisions here.

@Shadowfiend
Copy link
Contributor

For now, moving forward with 100% initial collateralization as the minimum (adding enforcement in a separate PR).

@Shadowfiend Shadowfiend merged commit aa8177e into master Mar 24, 2020
@Shadowfiend Shadowfiend deleted the base-liq-percent branch March 24, 2020 16:58
@Shadowfiend
Copy link
Contributor

Enforcement is in #541.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants